Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #333
Scan targets checked: wolfhsm-consttime, wolfhsm-crypto-bugs, wolfhsm-defaults, wolfhsm-mutation, wolfhsm-proptest, wolfhsm-src, wolfhsm-zeroize
No new issues found in the changed files. ✅
There was a problem hiding this comment.
As per PR description this was a false positive, but it's probably a better way to handle after the fixes.
Also, will hastype still needed after this PR?
A note:
- the fact that variants were never HSM-accellerated is a false claim, as the server port looks into the hashType and do accelleration if needed.
The fact that tests do not catch the bug because of the fallback mechanism is sad, we should probably address this in wolfHSM (@bigbrett ).
| * WC_HASH_TYPE_SHA512_224, or WC_HASH_TYPE_SHA512_256). | ||
| * @return int Returns 0 on success or a negative error code on failure. | ||
| */ | ||
| int wh_Client_Sha512(whClientContext* ctx, wc_Sha512* sha, const uint8_t* in, |
There was a problem hiding this comment.
This is an API break. I would prefer to add new specific function wh_Client_Sha512_224/wh_Client_Sha512_384
src/wh_client_crypto.c
Outdated
| /* Setup generic header and get pointer to request data */ | ||
| req = (whMessageCrypto_Sha2DmaRequest*)_createCryptoRequest( | ||
| dataPtr, WC_HASH_TYPE_SHA512, ctx->cryptoAffinity); | ||
| dataPtr, hashType, ctx->cryptoAffinity); |
There was a problem hiding this comment.
neither wh_Server_HandleCryptoDmaRequest nor wh_Server_HandleCryptoRequest hanlde SHA512 length variants (
wolfHSM/src/wh_server_crypto.c
Lines 4700 to 4711 in 752ca63
This request is never handled by the server.
Test fallback to client side, that's the reason tests are not failing.
There was a problem hiding this comment.
Good catch, fixed
| } | ||
|
|
||
| #if !defined(WOLFSSL_NOSHA512_224) | ||
| static int whTest_CryptoSha512_224(whClientContext* ctx, int devId, |
There was a problem hiding this comment.
a multiblock test is missing
| wc_Sha512 sha512[1]; | ||
| /* Buffer sized for the variant, plus canary to detect overwrite */ | ||
| uint8_t out[WC_SHA512_224_DIGEST_SIZE + 8]; | ||
| const uint8_t canary = 0xA5; |
There was a problem hiding this comment.
any reason why to rely on canary and not ASAN?
There was a problem hiding this comment.
I felt the programmatic approach was more direct. Can change if you feel strongly
padelsbach
force-pushed
the
sha512-alt-modes
branch
from
752ca63 to
15bd6f0
Compare
4 participants
Fixes F-2006 and F-2007. See related but independent wolfssl PR.
wh_Client_Sha512 unconditionally copied WC_SHA512_DIGEST_SIZE (64) bytes on finalize, regardless of SHA512 variant. For SHA512/224 (28-byte digest) and SHA512/256 (32-byte digest), this overwrites caller memory past the output buffer.
Today this is masked because the cryptocb dispatcher only handled WC_HASH_TYPE_SHA512, so variants fell through to CRYPTOCB_UNAVAILABLE and wolfSSL's software fallback handled the truncation safely. But the variants were never HSM-accelerated, and any future dispatcher fix would expose the overflow.
Changes
Design note
The hashType parameter comes from info->hash.type, which is set by wolfSSL's cryptocb dispatch layer based on digestSz -- not from sha512->hashType, which depends on the wolfSSL port's init code setting it. This makes the fix independent of wolfSSL port behavior.